-
Notifications
You must be signed in to change notification settings - Fork 206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: PRT: Add subscription metrics #1695
base: main
Are you sure you want to change the base?
Conversation
…dd-subscription-metrics
@@ -190,6 +202,7 @@ func (cwsm *ConsumerWSSubscriptionManager) StartSubscription( | |||
webSocketConnectionUniqueId string, | |||
metricsData *metrics.RelayMetrics, | |||
) (firstReply *pairingtypes.RelayReply, repliesChan <-chan *pairingtypes.RelayReply, err error) { | |||
go cwsm.rpcConsumerLogs.SetRelaySubscriptionRequestMetric(metricsData.ChainID, metricsData.APIType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we want this here. Users might spam with false subscription message, which will increase this metric for nothing. Maybe move this to after validation?
const ( | ||
ConsumerDisconnect RelayDisconnectReasonEnum = "ConsumerDisconnect" | ||
ProviderDisconnect RelayDisconnectReasonEnum = "ProviderDisconnect" | ||
UserDisconnect RelayDisconnectReasonEnum = "UserDisconnect" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this to metrics, change to int enum, and use map
…a into prt-add-subscription-metrics
@@ -201,6 +204,7 @@ func (cwsm *ConsumerWSSubscriptionManager) StartSubscription( | |||
utils.LogAttr("GUID", webSocketCtx), | |||
utils.LogAttr("hashedParams", utils.ToHexString(hashedParams)), | |||
utils.LogAttr("dappKey", dappKey), | |||
utils.LogAttr("connectedDapps", cwsm.connectedDapps), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is removed in main, why is is added back here?
const ( | ||
ConsumerDisconnect WsDisconnectReasonEnum = iota | ||
ProviderDisconnect WsDisconnectReasonEnum = iota | ||
UserDisconnect WsDisconnectReasonEnum = iota | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- You can do the
= iota
once in thins enum, as can be seen for example inprovider_optimizer.go
- Please use upper case snake case for this enum
- Please prefix the enum items with
WS_DISCONNECTION_REASON_
var disconnectReasonMap = map[WsDisconnectReasonEnum]string{ | ||
ConsumerDisconnect: "ConsumerDisconnect", | ||
ProviderDisconnect: "ProviderDisconnect", | ||
UserDisconnect: "UserDisconnect", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is only for logs, make the strings a bit more readable, like kebab-case
Help: "The total number of websocket subscription requests by the consumer over time per chain id per api interface.", | ||
}, []string{"spec", "apiInterface"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Help: "The total number of websocket subscription requests by the consumer over time per chain id per api interface.", | |
}, []string{"spec", "apiInterface"}) | |
Help: "The total number of websocket subscription requests over time per chain id per api interface.", | |
}, []string{"spec", "apiInterface"}) |
|
||
totalFailedWsSubscriptionRequestsMetric := prometheus.NewCounterVec(prometheus.CounterOpts{ | ||
Name: "lava_consumer_total_failed_ws_subscription_requests", | ||
Help: "The total number of failed websocket subscription requests by the consumer over time per chain id per api interface.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Help: "The total number of failed websocket subscription requests by the consumer over time per chain id per api interface.", | |
Help: "The total number of failed websocket subscription requests over time per chain id per api interface.", |
|
||
totalDuplicatedWsSubscriptionRequestsMetric := prometheus.NewCounterVec(prometheus.CounterOpts{ | ||
Name: "lava_consumer_total_duplicated_ws_subscription_requests", | ||
Help: "The total number of duplicated webscket subscription requests by the consumer over time per chain id per api interface.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Help: "The total number of duplicated webscket subscription requests by the consumer over time per chain id per api interface.", | |
Help: "The total number of duplicated webscket subscription requests over time per chain id per api interface.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Add consumer relay subscription metrics
https://github.com/orgs/Magma-Devs/projects/4/views/10?pane=issue&itemId=78426623